feat: deterministic close on the wrappers that own native state - #519
Open
milyin wants to merge 1 commit into
Open
feat: deterministic close on the wrappers that own native state#519milyin wants to merge 1 commit into
milyin wants to merge 1 commit into
Conversation
Three public wrappers directly own native memory whose lifetime escapes the call, but only some of them let a caller release it: - ZBytes holds a native buffer until first materialization, and is the one class here with no GC backstop (a per-message Cleaner costs -23% throughput), so an unread payload was unreleasable. It is now AutoCloseable with an explicit discard(): idempotent, taking the same monitor as the lazy materialization, so a concurrent read either wins (bytes stay readable) or loses (a later read fails with ZError instead of an NPE on handle!!). Discarding an already-materialized or user-created ZBytes is a no-op: the bytes stay readable. No Cleaner is added. - Config owns a Cleaner-backed native config; close() releases it eagerly and keeps the Cleaner as the forgotten-close fallback. Opening a session copies the config rather than taking it, so closing is purely the user's call. - Every SessionDeclaration implementation already had close() = undeclare(); the interface now declares it, so a value typed as SessionDeclaration works with try-with-resources / use. The existing overrides stay. Closes #518
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the lifecycle policy from #518: a public wrapper exposes
close()when it directly owns native state whose lifetime can escape the call.ZBytes— discardable and closeableZBytesnow implementsAutoCloseableand exposesdiscard(). It needs no new state field — the two existing fields already encode every state, andeager == null && handle == nullis "discarded":discard()toBytes()from(...))discard()is@Synchronizedon the same monitor as the lazy materialization, so read-vs-discard has two deterministic outcomes and never a use-after-free. The getter'shandle!!becamehandle ?: throw ZError(...), so a read after discard is a clear closed-state error rather than an NPE — that coverstoString/equals/hashCode/toZZBytestoo, which all route throughbytes. No Cleaner is added: the receive hot path is unchanged, andIntoZBytesis untouched.Config— AutoCloseableclose()delegates to the generated handle's already-idempotentclose(), keeping the Cleaner as the forgotten-close fallback.Zenoh.openuseszConfig.newClone(...)rather than consuming the config, so closing stays the user's decision; afterwardsgetJson/insertJson5/opening a session fail through the generated closed-handle guard asZError. The class KDoc said the config was "consumed by open" — corrected.SessionDeclaration : AutoCloseableclose() = undeclare()as a default. The six implementations (Publisher,Subscriber,Queryable,Querier,KeyExpr,LivelinessToken) already had exactly that override and keep it — this repo builds with Kotlin 1.9, where interface bodies compile toDefaultImpls, so leaving the per-class overrides in place is what keeps Java binary compatibility unconditional. The default is there for external/polymorphic use, as the issue asks.Out of scope per the issue:
Sample,Reply,Query,Encoding, and theInto*facades.Tests
New
ZBytesLifecycleTest.ktbuilds handle-backed instances directly (no session — the wrapper's state machine is what is under test) and covers: discard before materialization, discard after, repeatedclose()/discard(), a user-createdZBytes, and 500 rounds of concurrent read-vs-close asserting each read either returns the payload or throwsZError— nothing else. Plus one assertion thatSessionDeclarationisAutoCloseable.ConfigTestgains a double-close/use-after-close test../gradlew :zenoh-java:test -PuseLocalFlatJni=true— 119 tests, all green (-PuseLocalFlatJniis needed becauseorg.eclipse.zenoh:zenoh-flat-jni:1.9.0is not on Maven yet).Closes #518